Skip to content

Conversation

@WenyinWei
Copy link
Owner

@WenyinWei WenyinWei commented Jul 6, 2025

Implement a composable architecture for integrator facilities to enable flexible, decoupled feature combinations.

The previous design tightly coupled features like timeout and parallelism, leading to a combinatorial explosion of classes. This PR redesigns the architecture using a decorator pattern to ensure high cohesion and low coupling, allowing independent facilities (Timeout, Parallel, Async, Output, Signals) to be combined in any order without creating N*M combinations. This approach ensures future extensibility and maintainability.

Summary by Sourcery

Introduce comprehensive timeout protection and a composable decorator–based integration architecture into the core diffeq library, including independent facilities (timeout, parallel, async, output, signals) and a builder API for flexible composition; add a ParallelTimeoutIntegrator for automatic hardware-aware execution and zero-config convenience functions (integrate_auto, integrate_batch_auto); refactor core headers to expose new APIs; update tests to use timeout-protected integration with faster durations; and provide documentation and example programs illustrating the new features.

New Features:

  • Implement TimeoutIntegrator wrapper with configurable TimeoutConfig and IntegrationResult for production-ready timeout protection
  • Add ParallelTimeoutIntegrator for seamless, hardware-aware combination of timeout, parallel, and async execution
  • Introduce composable decorator facilities (TimeoutDecorator, ParallelDecorator, AsyncDecorator, OutputDecorator, SignalDecorator) and IntegratorBuilder for flexible feature stacking
  • Provide zero-configuration convenience APIs integrate_auto and integrate_batch_auto for automatic strategy selection

Enhancements:

  • Refactor diffeq.hpp to re-export core timeout, parallel-timeout, and composable integration APIs
  • Update unit and integration tests to use integrate_with_timeout and reduce test durations for faster, robust CI
  • Add example programs demonstrating timeout integration, seamless parallel-timeout execution, and composable facilities

Documentation:

  • Add COMPOSABLE_ARCHITECTURE.md and multiple summary documents detailing the new architecture and usage patterns
  • Update examples/README.md to include timeout and composable integration demos

Tests:

  • Enhance tests to assert timeout completion and leverage new timeout API in Lorenz, performance, DOP853, and modernized interface tests

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jul 6, 2025

Reviewer's Guide

This PR overhauls the integrator facility architecture by introducing a decorator‐based, composable model (Timeout, Parallel, Async, Output, Signal) that replaces the previous combinatorial class explosion. It adds new core timeout and parallel–timeout integrators with automatic hardware detection and strategy selection, refactors tests and examples to use the unified diffeq.hpp interface and built-in timeout protections, and enriches the documentation with targeted demos and design guides.

Class diagram for the new composable integrator architecture

classDiagram
    class AbstractIntegrator {
        <<interface>>
        +step(state, dt)
        +integrate(state, dt, end_time)
        +current_time()
        +set_time(t)
        +set_system(sys)
    }
    class IntegratorDecorator {
        +wrapped_integrator_: AbstractIntegrator
        +step(state, dt)
        +integrate(state, dt, end_time)
        +current_time()
        +set_time(t)
        +set_system(sys)
        +wrapped()
    }
    class TimeoutDecorator {
        +config_: TimeoutConfig
        +integrate_with_timeout(state, dt, end_time)
    }
    class ParallelDecorator {
        +config_: ParallelConfig
        +integrate_batch(states, dt, end_time)
        +integrate_monte_carlo(num_simulations, generator, processor, dt, end_time)
    }
    class AsyncDecorator {
        +config_: AsyncConfig
        +integrate_async(state, dt, end_time)
        +step_async(state, dt)
    }
    class OutputDecorator {
        +config_: OutputConfig
        +set_output_handler(handler)
        +get_buffer()
        +clear_buffer()
    }
    class SignalDecorator {
        +config_: SignalConfig
        +register_signal_handler(handler)
        +clear_signal_handlers()
    }
    class IntegratorBuilder {
        +with_timeout(config)
        +with_parallel(config)
        +with_async(config)
        +with_output(config, handler)
        +with_signals(config)
        +build()
    }
    AbstractIntegrator <|-- IntegratorDecorator
    IntegratorDecorator <|-- TimeoutDecorator
    IntegratorDecorator <|-- ParallelDecorator
    IntegratorDecorator <|-- AsyncDecorator
    IntegratorDecorator <|-- OutputDecorator
    IntegratorDecorator <|-- SignalDecorator
    class TimeoutConfig
    class ParallelConfig
    class AsyncConfig
    class OutputConfig
    class SignalConfig
    IntegratorBuilder --> AbstractIntegrator : builds
    TimeoutDecorator --> TimeoutConfig
    ParallelDecorator --> ParallelConfig
    AsyncDecorator --> AsyncConfig
    OutputDecorator --> OutputConfig
    SignalDecorator --> SignalConfig
Loading

Class diagram for ParallelTimeoutIntegrator and related types

classDiagram
    class ParallelTimeoutIntegrator {
        +base_integrator_: Integrator
        +async_integrator_: AsyncIntegrator
        +integration_interface_: IntegrationInterface
        +config_: ParallelTimeoutConfig
        +hardware_caps_: HardwareCapabilities
        +integrate_with_auto_parallel(state, dt, end_time)
        +integrate_batch(states, dt, end_time)
        +integrate_monte_carlo(num_simulations, generator, processor, dt, end_time)
        +integrate_realtime(state, dt, end_time)
        +base_integrator()
        +async_integrator()
        +integration_interface()
        +config()
        +hardware_capabilities()
    }
    class ParallelTimeoutConfig {
        +timeout_config: TimeoutConfig
        +strategy: ExecutionStrategy
        +performance_hint: PerformanceHint
        +max_parallel_tasks
        +chunk_size
        +async_thread_pool_size
        +enable_async_stepping
        +enable_state_monitoring
        +enable_hardware_detection
        +prefer_async_over_parallel
        +parallel_threshold
        +enable_signal_processing
        +signal_check_interval
    }
    class ParallelIntegrationResult {
        +timeout_result: IntegrationResult
        +used_strategy: ExecutionStrategy
        +parallel_tasks_used
        +setup_time
        +execution_time
        +teardown_time
        +hardware_used: HardwareCapabilities
        +is_success()
        +is_timeout()
        +total_elapsed_time()
    }
    class HardwareCapabilities {
        +cpu_cores
        +has_gpu
        +supports_simd
        +supports_std_execution
        +sequential_performance_score
        +parallel_performance_score
        +async_performance_score
        +detect()
    }
    class ExecutionStrategy
    class PerformanceHint
    class TimeoutConfig
    class IntegrationResult
    ParallelTimeoutIntegrator --> ParallelTimeoutConfig
    ParallelTimeoutIntegrator --> HardwareCapabilities
    ParallelTimeoutIntegrator --> ParallelIntegrationResult
    ParallelTimeoutConfig --> TimeoutConfig
    ParallelIntegrationResult --> IntegrationResult
    ParallelIntegrationResult --> HardwareCapabilities
    ParallelTimeoutConfig --> ExecutionStrategy
    ParallelTimeoutConfig --> PerformanceHint
Loading

Class diagram for TimeoutIntegrator and IntegrationResult

classDiagram
    class TimeoutIntegrator {
        +integrator_: Integrator
        +config_: TimeoutConfig
        +integrate_with_timeout(state, dt, end_time)
        +integrate_with_timeout_simple(state, dt, end_time, timeout)
        +integrator()
        +config()
    }
    class TimeoutConfig {
        +timeout_duration
        +throw_on_timeout
        +enable_progress_callback
        +progress_interval
        +progress_callback
    }
    class IntegrationResult {
        +completed
        +elapsed_time
        +final_time
        +error_message
        +is_success()
        +is_timeout()
        +is_error()
    }
    TimeoutIntegrator --> TimeoutConfig
    TimeoutIntegrator --> IntegrationResult
Loading

File-Level Changes

Change Details Files
Introduce composable integration architecture via decorator pattern
  • Add composable_integration.hpp with independent Timeout, Parallel, Async, Output, Signal decorators and a builder interface
  • Update diffeq.hpp to re-export composable integration APIs
  • Enable order-independent composition of facilities
include/core/composable_integration.hpp
include/diffeq.hpp
Add ParallelTimeoutIntegrator for seamless timeout + async + parallel execution
  • Implement ParallelTimeoutIntegrator with automatic strategy selection, batch and Monte Carlo support
  • Re-export parallel timeout types and factory functions in diffeq.hpp
  • Provide seamless_parallel_timeout_demo.cpp example showcasing zero-config integration
include/core/parallel_timeout_integrator.hpp
include/diffeq.hpp
examples/seamless_parallel_timeout_demo.cpp
Implement TimeoutIntegrator for timeout-protected integration
  • Add TimeoutIntegrator wrapper and TimeoutConfig, IntegrationResult, exception support in timeout_integrator.hpp
  • Expose make_timeout_integrator and integrate_with_timeout in diffeq.hpp
  • Include timeout_integration_demo.cpp example demonstrating basic and advanced timeout usage
include/core/timeout_integrator.hpp
include/diffeq.hpp
examples/timeout_integration_demo.cpp
Refactor tests to use unified interface and built-in timeout
  • Replace individual integrator includes with single diffeq.hpp
  • Use integrate_with_timeout and TimeoutIntegrator in unit and integration tests
  • Reduce test durations, relax tolerances, and switch to ASSERT_TRUE on completion
  • Adjust test code to remove redundant EXPECT_NO_THROW blocks
test/unit/test_advanced_integrators.cpp
test/unit/test_dop853.cpp
test/integration/test_modernized_interface.cpp
Expand examples and documentation with new demos and guides
  • Add new composable, seamless parallel+timeout, and timeout demo programs
  • Update examples/README.md with best practices and facility listings
  • Introduce architecture and integration summaries in markdown (COMPOSABLE_ARCHITECTURE.md, SEAMLESS_INTEGRATION_SUMMARY.md, TIMEOUT_INTEGRATION_SUMMARY.md)
examples/composable_facilities_demo.cpp
examples/timeout_integration_demo.cpp
examples/seamless_parallel_timeout_demo.cpp
examples/README.md
COMPOSABLE_ARCHITECTURE.md
SEAMLESS_INTEGRATION_SUMMARY.md
TIMEOUT_INTEGRATION_SUMMARY.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @WenyinWei - I've reviewed your changes - here's some feedback:

  • The create_integrator_copy stub in ParallelTimeoutIntegrator will throw at runtime—add a proper integrator‐copy mechanism or factory to ensure parallel/batch methods work without error.
  • ParallelTimeoutIntegrator is very large and mixes strategy selection with execution details—consider extracting each execution strategy (sequential, parallel, async, hybrid) into its own class or helper to improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `create_integrator_copy` stub in `ParallelTimeoutIntegrator` will throw at runtime—add a proper integrator‐copy mechanism or factory to ensure parallel/batch methods work without error.
- `ParallelTimeoutIntegrator` is very large and mixes strategy selection with execution details—consider extracting each execution strategy (sequential, parallel, async, hybrid) into its own class or helper to improve readability and maintainability.

## Individual Comments

### Comment 1
<location> `include/core/parallel_timeout_integrator.hpp:491` </location>
<code_context>
+        return std::min(config_.max_parallel_tasks, hardware_caps_.cpu_cores);
+    }
+
+    std::unique_ptr<Integrator> create_integrator_copy() {
+        // This is a placeholder - actual implementation would depend on integrator type
+        // Could use a factory pattern or require integrators to be copyable
+        throw std::runtime_error("Integrator copying not implemented - need factory pattern");
+    }
+
</code_context>

<issue_to_address>
The create_integrator_copy method is unimplemented and throws at runtime.

This stub will cause runtime failures in parallel or batch operations unless users implement their own copy logic. To prevent such errors, require integrators to be copyable or provide a factory/copy mechanism in the interface.
</issue_to_address>

### Comment 2
<location> `include/core/parallel_timeout_integrator.hpp:497` </location>
<code_context>
+        throw std::runtime_error("Integrator copying not implemented - need factory pattern");
+    }
+
+    std::unique_ptr<ParallelTimeoutIntegrator> create_thread_local_integrator() {
+        // Create a new integrator for thread-local use
+        // This is needed for true parallelization
+        auto local_config = config_;
+        local_config.strategy = ExecutionStrategy::SEQUENTIAL;  // Avoid nested parallelization
+        
+        return std::make_unique<ParallelTimeoutIntegrator>(
+            create_integrator_copy(), local_config);
+    }
+};
</code_context>

<issue_to_address>
Thread-local integrator creation depends on create_integrator_copy, which is not implemented.

This dependency causes runtime exceptions when using batch or Monte Carlo integration, making the parallel API appear functional when it is not. Please document this limitation clearly or refactor to prevent misuse.
</issue_to_address>

### Comment 3
<location> `include/core/parallel_timeout_integrator.hpp:258` </location>
<code_context>
+        using result_type = std::invoke_result_t<Processor, S>;
+        std::vector<result_type> results(num_simulations);
+
+        std::for_each(std::execution::par_unseq,
+            std::views::iota(0UL, num_simulations).begin(),
+            std::views::iota(0UL, num_simulations).end(),
+            [&](size_t i) {
+                auto local_integrator = this->create_copy();
+                auto state = generator(i);
</code_context>

<issue_to_address>
Use of std::execution::par_unseq assumes hardware and compiler support.

Check hardware_caps_.supports_std_execution before using par_unseq, or provide a fallback to par or sequential execution if unsupported.
</issue_to_address>

### Comment 4
<location> `include/core/parallel_timeout_integrator.hpp:399` </location>
<code_context>
+        }
+        
+        // Estimate problem characteristics
+        double integration_steps = (end_time - 0) / dt;  // Assuming start time is 0
+        double problem_size = state.size() * integration_steps;
+        
+        // Select strategy based on problem characteristics and hardware
</code_context>

<issue_to_address>
Assumes start time is always zero when estimating integration steps.

This may result in incorrect step estimates if integration starts at a nonzero time. Use the actual start time from the integrator or state to improve accuracy.

Suggested implementation:

```
    ExecutionStrategy select_execution_strategy(
        const state_type& state, 
        time_type dt, 
        time_type start_time,
        time_type end_time
    ) {
        if (config_.strategy != ExecutionStrategy::AUTO) {
            return config_.strategy;
        }

        // Estimate problem characteristics
        double integration_steps = (end_time - start_time) / dt;
        double problem_size = state.size() * integration_steps;

        // Select strategy based on problem characteristics and hardware

```

You will need to update all call sites of `select_execution_strategy` to pass the correct `start_time` value as an argument. This may require retrieving the start time from the integrator or state, depending on your codebase.
</issue_to_address>

### Comment 5
<location> `include/core/timeout_integrator.hpp:50` </location>
<code_context>
+    std::string error_message;
+    
+    bool is_success() const { return completed && error_message.empty(); }
+    bool is_timeout() const { return !completed && error_message.find("timeout") != std::string::npos; }
+};
+
</code_context>

<issue_to_address>
Timeout detection relies on error message string matching.

Checking for the substring "timeout" in error messages is unreliable and may cause incorrect detection if messages change or are localized. Use a status field or error code for timeout detection instead.

Suggested implementation:

```
enum class IntegrationStatus {
    Success,
    Timeout,
    Failure
};

struct IntegrationResult {
    bool completed{false};                          // Whether integration completed successfully
    std::chrono::milliseconds elapsed_time{0};     // Total elapsed time
    double final_time{0.0};                        // Final integration time reached
    std::string error_message;                     // Error message if failed
    IntegrationStatus status{IntegrationStatus::Success}; // Explicit status

    // Success indicators
    bool is_success() const { return status == IntegrationStatus::Success; }
    bool is_timeout() const { return status == IntegrationStatus::Timeout; }
};

```

You will need to update all code that creates or modifies `IntegrationResult` to set the `status` field appropriately (e.g., set `status = IntegrationStatus::Timeout` when a timeout occurs, and `status = IntegrationStatus::Failure` for other errors).
</issue_to_address>

### Comment 6
<location> `include/core/timeout_integrator.hpp:155` </location>
<code_context>
+     * @param timeout Optional timeout override
+     * @return true if completed successfully, false if timed out
+     */
+    bool integrate_with_timeout_simple(state_type& state, time_type dt, time_type end_time,
+                                      std::chrono::milliseconds timeout = std::chrono::milliseconds{0}) {
+        auto old_config = config_;
+        if (timeout.count() > 0) {
+            config_.timeout_duration = timeout;
+        }
+        config_.throw_on_timeout = false;
+        
+        auto result = integrate_with_timeout(state, dt, end_time);
+        config_ = old_config; // Restore original config
+        
+        return result.completed;
+    }
+
</code_context>

<issue_to_address>
integrate_with_timeout_simple ignores error type and only returns completion.

Currently, this method only indicates if the integration completed, not whether it succeeded or failed due to an error (other than timeout). Consider returning more detailed status information or checking is_success() to avoid masking non-timeout failures.

Suggested implementation:

```
     * @param timeout Optional timeout override
     * @return IntegrationResult containing completion and success status
     */
    IntegrationResult integrate_with_timeout_simple(state_type& state, time_type dt, time_type end_time,
                                      std::chrono::milliseconds timeout = std::chrono::milliseconds{0}) {
        auto old_config = config_;
        if (timeout.count() > 0) {
            config_.timeout_duration = timeout;
        }
        config_.throw_on_timeout = false;

        auto result = integrate_with_timeout(state, dt, end_time);
        config_ = old_config; // Restore original config

        return result;
    }

```

- Ensure that `IntegrationResult` (or the actual result type returned by `integrate_with_timeout`) is accessible in this context and documents both completion and success/error status.
- Update any code that calls `integrate_with_timeout_simple` to handle the new return type.
- If you want to expose only a subset of the result (e.g., a struct with `completed` and `success`), you can define and return such a struct instead.
</issue_to_address>

### Comment 7
<location> `include/core/composable_integration.hpp:358` </location>
<code_context>
+        }
+    }
+
+    void buffer_output(const S& state, T time) {
+        if (output_buffer_.size() >= config_.buffer_size) {
+            output_buffer_.erase(output_buffer_.begin());
+        }
+        output_buffer_.emplace_back(state, time);
+    }
+
</code_context>

<issue_to_address>
Output buffer uses erase(begin) for buffer overflow, which is O(n).

Using erase(begin) on std::vector is O(n) and may degrade performance with large buffers. Consider a circular buffer or std::deque for more efficient removal of the oldest element.

Suggested implementation:

```
#include <deque>

```

```
        if (output_buffer_.size() >= config_.buffer_size) {
            output_buffer_.erase(output_buffer_.begin());
        }
        output_buffer_.emplace_back(state, time);

```

You must also change the declaration of `output_buffer_` from `std::vector` to `std::deque` in the class definition (not shown in the provided code). For example:
`std::vector<...> output_buffer_;`
should become
`std::deque<...> output_buffer_;`
</issue_to_address>

### Comment 8
<location> `test/unit/test_advanced_integrators.cpp:209` </location>
<code_context>
         integrator.set_time(0.0);
-        EXPECT_NO_THROW(integrator.integrate(y, dt, t_end));
+        
+        bool completed = diffeq::integrate_with_timeout(integrator, y, dt, t_end, TIMEOUT);
+        ASSERT_TRUE(completed) << "RK4 integration timed out after " << TIMEOUT.count() << " seconds";
+        
         // Just check solution is bounded (Lorenz attractor is bounded)
</code_context>

<issue_to_address>
Consider adding tests for timeout expiration and error handling.

Add a test where the integration exceeds the timeout to ensure the timeout mechanism triggers and the test fails as expected.
</issue_to_address>

### Comment 9
<location> `test/integration/test_modernized_interface.cpp:325` </location>
<code_context>
-            auto future = async_integrator->integrate_async(initial_state, 0.01, 1.0);
+            auto future = async_integrator->integrate_async(initial_state, 0.01, 0.5);  // Reduced from 1.0 to 0.5 seconds
+            
+            // Wait for completion with timeout
+            const std::chrono::seconds TIMEOUT{3};
+            if (future.wait_for(TIMEOUT) == std::future_status::timeout) {
+                std::cout << "     Async integration timed out after " << TIMEOUT.count() << " seconds" << std::endl;
+                return false;
</code_context>

<issue_to_address>
Missing test for async timeout failure path.

Please add a test that triggers a timeout to verify correct handling of async integration timeouts.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
            auto future = async_integrator->integrate_async(initial_state, 0.01, 0.5);  // Reduced from 1.0 to 0.5 seconds

            // Wait for completion with timeout
            const std::chrono::seconds TIMEOUT{3};
            if (future.wait_for(TIMEOUT) == std::future_status::timeout) {
                std::cout << "     Async integration timed out after " << TIMEOUT.count() << " seconds" << std::endl;
                return false;
            }

            future.wait();
            std::cout << "     Async integration completed: ✓" << std::endl;
=======
            auto future = async_integrator->integrate_async(initial_state, 0.01, 0.5);  // Reduced from 1.0 to 0.5 seconds

            // Wait for completion with timeout
            const std::chrono::seconds TIMEOUT{3};
            if (future.wait_for(TIMEOUT) == std::future_status::timeout) {
                std::cout << "     Async integration timed out after " << TIMEOUT.count() << " seconds" << std::endl;
                return false;
            }

            future.wait();
            std::cout << "     Async integration completed: ✓" << std::endl;

            // --- Test: Async integration timeout failure path ---
            {
                std::vector<double> timeout_state = {1.0, 0.0};
                // Set integration duration much longer than timeout to force timeout
                auto timeout_future = async_integrator->integrate_async(timeout_state, 0.01, 10.0);
                const std::chrono::seconds SHORT_TIMEOUT{1};
                if (timeout_future.wait_for(SHORT_TIMEOUT) == std::future_status::timeout) {
                    std::cout << "     [TEST] Async integration timeout failure path triggered as expected after "
                              << SHORT_TIMEOUT.count() << " seconds" << std::endl;
                } else {
                    std::cerr << "     [TEST] ERROR: Async integration did not timeout as expected" << std::endl;
                    return false;
                }
            }
            // --- End test ---
>>>>>>> REPLACE

</suggested_fix>

### Comment 10
<location> `test/integration/test_modernized_interface.cpp:371` </location>
<code_context>
+        auto end_time = std::chrono::high_resolution_clock::now();
+        
+        auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end_time - start_time);
+        if (duration.count() > 2000) {  // 2-second timeout
+            std::cout << "     Signal-aware integration took too long: " << duration.count() << "ms" << std::endl;
+            return false;
+        }

</code_context>

<issue_to_address>
Consider asserting on the return value to ensure test failure on timeout.

Using an assertion on the duration will cause the test to fail if the timeout is exceeded, making the failure explicit and the test's intent clearer.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end_time - start_time);
        if (duration.count() > 2000) {  // 2-second timeout
            std::cout << "     Signal-aware integration took too long: " << duration.count() << "ms" << std::endl;
            return false;
        }
=======
        auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end_time - start_time);
        ASSERT_LE(duration.count(), 2000) << "Signal-aware integration took too long: " << duration.count() << "ms";
>>>>>>> REPLACE

</suggested_fix>

### Comment 11
<location> `test/unit/test_dop853.cpp:231` </location>
<code_context>

     try {
-        integrator.integrate(y, 0.01, 1.0);
+        bool completed = diffeq::integrate_with_timeout(integrator, y, 0.01, 0.5, TIMEOUT);  // Reduced from 1.0 to 0.5 seconds
+        ASSERT_TRUE(completed) << "DOP853 performance test timed out after " << TIMEOUT.count() << " seconds";

         auto end_time = std::chrono::high_resolution_clock::now();
</code_context>

<issue_to_address>
No test for timeout expiration with DOP853 integrator.

Please add a test that ensures the DOP853 integrator triggers and handles a timeout as expected, such as by using a stiff system or a very short timeout value.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 491 to 494
std::unique_ptr<Integrator> create_integrator_copy() {
// This is a placeholder - actual implementation would depend on integrator type
// Could use a factory pattern or require integrators to be copyable
throw std::runtime_error("Integrator copying not implemented - need factory pattern");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): The create_integrator_copy method is unimplemented and throws at runtime.

This stub will cause runtime failures in parallel or batch operations unless users implement their own copy logic. To prevent such errors, require integrators to be copyable or provide a factory/copy mechanism in the interface.

Comment on lines 497 to 504
std::unique_ptr<ParallelTimeoutIntegrator> create_thread_local_integrator() {
// Create a new integrator for thread-local use
// This is needed for true parallelization
auto local_config = config_;
local_config.strategy = ExecutionStrategy::SEQUENTIAL; // Avoid nested parallelization

return std::make_unique<ParallelTimeoutIntegrator>(
create_integrator_copy(), local_config);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Thread-local integrator creation depends on create_integrator_copy, which is not implemented.

This dependency causes runtime exceptions when using batch or Monte Carlo integration, making the parallel API appear functional when it is not. Please document this limitation clearly or refactor to prevent misuse.

Comment on lines 258 to 261
std::for_each(std::execution::par_unseq,
std::views::iota(0UL, batch_size).begin(),
std::views::iota(0UL, batch_size).end(),
[&](size_t i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Use of std::execution::par_unseq assumes hardware and compiler support.

Check hardware_caps_.supports_std_execution before using par_unseq, or provide a fallback to par or sequential execution if unsupported.

Comment on lines 399 to 400
double integration_steps = (end_time - 0) / dt; // Assuming start time is 0
double problem_size = state.size() * integration_steps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Assumes start time is always zero when estimating integration steps.

This may result in incorrect step estimates if integration starts at a nonzero time. Use the actual start time from the integrator or state to improve accuracy.

Suggested implementation:

    ExecutionStrategy select_execution_strategy(
        const state_type& state, 
        time_type dt, 
        time_type start_time,
        time_type end_time
    ) {
        if (config_.strategy != ExecutionStrategy::AUTO) {
            return config_.strategy;
        }

        // Estimate problem characteristics
        double integration_steps = (end_time - start_time) / dt;
        double problem_size = state.size() * integration_steps;

        // Select strategy based on problem characteristics and hardware

You will need to update all call sites of select_execution_strategy to pass the correct start_time value as an argument. This may require retrieving the start time from the integrator or state, depending on your codebase.


// Success indicators
bool is_success() const { return completed && error_message.empty(); }
bool is_timeout() const { return !completed && error_message.find("timeout") != std::string::npos; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Timeout detection relies on error message string matching.

Checking for the substring "timeout" in error messages is unreliable and may cause incorrect detection if messages change or are localized. Use a status field or error code for timeout detection instead.

Suggested implementation:

enum class IntegrationStatus {
    Success,
    Timeout,
    Failure
};

struct IntegrationResult {
    bool completed{false};                          // Whether integration completed successfully
    std::chrono::milliseconds elapsed_time{0};     // Total elapsed time
    double final_time{0.0};                        // Final integration time reached
    std::string error_message;                     // Error message if failed
    IntegrationStatus status{IntegrationStatus::Success}; // Explicit status

    // Success indicators
    bool is_success() const { return status == IntegrationStatus::Success; }
    bool is_timeout() const { return status == IntegrationStatus::Timeout; }
};

You will need to update all code that creates or modifies IntegrationResult to set the status field appropriately (e.g., set status = IntegrationStatus::Timeout when a timeout occurs, and status = IntegrationStatus::Failure for other errors).

Comment on lines 358 to 362
void buffer_output(const S& state, T time) {
if (output_buffer_.size() >= config_.buffer_size) {
output_buffer_.erase(output_buffer_.begin());
}
output_buffer_.emplace_back(state, time);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (performance): Output buffer uses erase(begin) for buffer overflow, which is O(n).

Using erase(begin) on std::vector is O(n) and may degrade performance with large buffers. Consider a circular buffer or std::deque for more efficient removal of the oldest element.

Suggested implementation:

#include <deque>

        if (output_buffer_.size() >= config_.buffer_size) {
            output_buffer_.erase(output_buffer_.begin());
        }
        output_buffer_.emplace_back(state, time);

You must also change the declaration of output_buffer_ from std::vector to std::deque in the class definition (not shown in the provided code). For example:
std::vector<...> output_buffer_;
should become
std::deque<...> output_buffer_;

Comment on lines +209 to +210
bool completed = diffeq::integrate_with_timeout(integrator, y, dt, t_end, TIMEOUT);
ASSERT_TRUE(completed) << "RK4 integration timed out after " << TIMEOUT.count() << " seconds";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding tests for timeout expiration and error handling.

Add a test where the integration exceeds the timeout to ensure the timeout mechanism triggers and the test fails as expected.

Comment on lines +323 to 333
auto future = async_integrator->integrate_async(initial_state, 0.01, 0.5); // Reduced from 1.0 to 0.5 seconds

// Wait for completion with timeout
const std::chrono::seconds TIMEOUT{3};
if (future.wait_for(TIMEOUT) == std::future_status::timeout) {
std::cout << " Async integration timed out after " << TIMEOUT.count() << " seconds" << std::endl;
return false;
}

// Wait for completion
future.wait();
std::cout << " Async integration completed: ✓" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Missing test for async timeout failure path.

Please add a test that triggers a timeout to verify correct handling of async integration timeouts.

Suggested change
auto future = async_integrator->integrate_async(initial_state, 0.01, 0.5); // Reduced from 1.0 to 0.5 seconds
// Wait for completion with timeout
const std::chrono::seconds TIMEOUT{3};
if (future.wait_for(TIMEOUT) == std::future_status::timeout) {
std::cout << " Async integration timed out after " << TIMEOUT.count() << " seconds" << std::endl;
return false;
}
// Wait for completion
future.wait();
std::cout << " Async integration completed: ✓" << std::endl;
auto future = async_integrator->integrate_async(initial_state, 0.01, 0.5); // Reduced from 1.0 to 0.5 seconds
// Wait for completion with timeout
const std::chrono::seconds TIMEOUT{3};
if (future.wait_for(TIMEOUT) == std::future_status::timeout) {
std::cout << " Async integration timed out after " << TIMEOUT.count() << " seconds" << std::endl;
return false;
}
future.wait();
std::cout << " Async integration completed: ✓" << std::endl;
// --- Test: Async integration timeout failure path ---
{
std::vector<double> timeout_state = {1.0, 0.0};
// Set integration duration much longer than timeout to force timeout
auto timeout_future = async_integrator->integrate_async(timeout_state, 0.01, 10.0);
const std::chrono::seconds SHORT_TIMEOUT{1};
if (timeout_future.wait_for(SHORT_TIMEOUT) == std::future_status::timeout) {
std::cout << " [TEST] Async integration timeout failure path triggered as expected after "
<< SHORT_TIMEOUT.count() << " seconds" << std::endl;
} else {
std::cerr << " [TEST] ERROR: Async integration did not timeout as expected" << std::endl;
return false;
}
}
// --- End test ---

Comment on lines +231 to +232
bool completed = diffeq::integrate_with_timeout(integrator, y, 0.01, 0.5, TIMEOUT); // Reduced from 1.0 to 0.5 seconds
ASSERT_TRUE(completed) << "DOP853 performance test timed out after " << TIMEOUT.count() << " seconds";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): No test for timeout expiration with DOP853 integrator.

Please add a test that ensures the DOP853 integrator triggers and handles a timeout as expected, such as by using a stiff system or a very short timeout value.

WenyinWei and others added 3 commits July 6, 2025 21:09
… timeout is exceeded, making the failure explicit and the test's intent clearer.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@WenyinWei WenyinWei merged commit b9dd7bb into main Jul 6, 2025
1 of 3 checks passed
@WenyinWei WenyinWei deleted the cursor/set-timeout-limits-for-difficult-tests-4ae4 branch July 6, 2025 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants